✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name	Link
🔨 Latest commit	4af2ff4
🔍 Latest deploy log	https://app.netlify.com/projects/kubernetes-sigs-cluster-api-openstack/deploys/688c7d24fb347600089077a5
😎 Deploy Preview	https://deploy-preview-2628--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Hi @bnallapeta. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

I'm very much in favour of this. A few API-related notes inline.

The more I think about it, the more I'm convinced we should extend identityRef rather than adding an alternative with a fallback. I think the semantics and validation will be much clearer.

Incidentally, on the subject of validations I'd expect this to be validated with CEL. Lets not add anything new to the existing webhooks when it's not required. That's an implementation detail, though, so no need to add CEL expressions to this doc unless you really want to.

yes, I considered both while thinking about this proposal. Let me put down my thoughts as simple pros vs cons:

new clusterIdentityRef
pros:
zero risk to existing APIs
validation becomes easier and cleaner
fallback designed to support both
migration would be easier
naming is in par with other providers - users feel familiar

cons:
dual fields for the same purpose - might be confusing to users
maintenance burden

extend current identityRef
pros:
users don't have to learn about a new field, less confusing
cleaner long-term API

cons:
higher migration risk
have to consider migration scenarios for testing

Kindly think this over and let me know.

cons: higher migration risk have to consider migration scenarios for testing

I don't follow this. Can you explain the additional risk?

For additional context I'm thinking something like this:

Current

This is the existing syntax. It is identical, with identical semantics. The absence of a type field means we default to Secret.

apiVersion: infrastructure.cluster.x-k8s.io/v1beta1
kind: OpenStackCluster
metadata:
  name: my-cluster
  namespace: team-a
spec:
  identityRef:
    name: fallback-secret
    cloudName: openstack

ClusterIdentity

Specifying a ClusterIdentity. Note the type field.

apiVersion: infrastructure.cluster.x-k8s.io/v1beta1
kind: OpenStackCluster
metadata:
  name: my-cluster
  namespace: team-a
spec:
  identityRef:
    type: ClusterIdentity
    name: production-openstack

Current with explicit type

This form uses the current Secret semantics explicitly.

apiVersion: infrastructure.cluster.x-k8s.io/v1beta1
kind: OpenStackCluster
metadata:
  name: my-cluster
  namespace: team-a
spec:
  identityRef:
    type: Secret
    name: fallback-secret
    cloudName: openstack

This is a common pattern in kubernetes APIs. It's not strictly a discriminated union because that typically discriminates on a single sub-field, but it's close enough without breaking backwards compatibility. This pattern has a number of advantages:

• It is very easy to validate: the validation is limited in scope to a single struct (in this case identityRef).
• It is easy to implement without breaking backwards compatibility.
• It is easy to extend in the future.
• It is unambiguous: only a single thing can be expressed.

Incidentally, an older version of the CAPO API actually had a type field, but we removed it because we only had a single type. Perhaps that was short-sighted, but it's easy enough to add back.

Another reason in favour of extending the existing struct is that it would propagate everywhere that the struct is used, including OpenStackMachine (and Server). We have had users relying on separate Machine credentials, and there's no reason to restrict which credential types they can use.

/lgtm

@lentzi90 @EmilienM Any thoughts? This is quite a big change, but I think it's a good one.

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lentzi90

The full list of commands accepted by this bot can be found here.

The pull request process is described here

@mandre There's definitely an impact on ORC. ORC will need to implement this or something like it.